Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve sorting and displaying algorithms for top-clients and top-domains #196

Merged
merged 6 commits into from
Jan 11, 2018

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jan 8, 2018

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10


  • Implement improved algorithm in getTopDomains(). Instead of assuming how many lines of data we have sent, we can also explicitly count them.
  • Implement improved algorithm in getTopClients(). As a by-product this also adds the optional desc feature to the top clients requests where it wasn't available before.

Note that I completely removed the skip variable as it is not important how many lines we have already skipped when now directly count the number of returned datasets.

The new code has been tested against two possible failures:

  • Requested more data than is available, and
  • Requested data when no data is available.

Both tests returned the expected results.

This template was created based on the work of udemy-dl.

…how many lines of data we have sent, we can also explicitly count them.

Signed-off-by: DL6ER <dl6er@dl6er.de>
… also adds the optional "desc" feature to the top clients requests where it wasn't available before

Signed-off-by: DL6ER <dl6er@dl6er.de>
continue;
}
}
if(excludedomains != NULL && insetupVarsArray(domains[j].domain))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using short-circuit evaluation here, no functional change.

continue;
}
}
if(excludeclients != NULL && (insetupVarsArray(clients[j].ip) || insetupVarsArray(clients[j].name)))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using short-circuit evaluation here, no functional change.

request.c Outdated
// Sort temporary array
qsort(temparray, counters.clients, sizeof(int[2]), cmpasc);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added optional desc sorting here, this is basically a copy of the code in lines 369-372.

@AzureMarker
Copy link
Contributor

desc should be renamed to asc for ascending, because >top-domains is descending already and >top-domains desc is ascending.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
Copy link
Member Author

DL6ER commented Jan 8, 2018

Okay, again ready for review. Clarified the roles of ascending and descending and added three additional test that should ensure that we don't mix it up, again.

@AzureMarker AzureMarker merged commit 561c55e into development Jan 11, 2018
@AzureMarker AzureMarker deleted the fix/desc_sorting branch January 11, 2018 03:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants